Skip to content

RemoteEdit Draft Management and Recovery - #241

Open
nschimme wants to merge 7 commits into
masterfrom
remote-edit-draft-management-18138830753454044249
Open

RemoteEdit Draft Management and Recovery#241
nschimme wants to merge 7 commits into
masterfrom
remote-edit-draft-management-18138830753454044249

Conversation

@nschimme

@nschimme nschimme commented Jul 5, 2026

Copy link
Copy Markdown
Owner

This change implements the RemoteEdit Draft Management and Recovery Agent. It refactors the existing RemoteEdit system to centralize draft persistence and session management. MainWindow now owns the long-lived RemoteEdit instance, and Proxy is decoupled via signals/slots. Drafts are automatically saved with debounce and throttle logic, and orphaned drafts are recovered as background tasks upon application startup. recovered drafts can be inspected in a read-only viewer.


PR created automatically by Jules for task 18138830753454044249 started by @nschimme

Summary by Sourcery

Centralize RemoteEdit lifecycle in MainWindow, introduce persistent draft storage with recovery, and integrate edit tasks into the async task and UI systems.

New Features:

  • Persist remote editor drafts to a configurable editor directory with debounced and throttled autosave.
  • Automatically discover and register orphaned draft files as recovered, non-sendable RemoteEdit tasks that can be inspected in a read-only viewer.
  • Expose RemoteEdit tasks in the Tasks panel with an action to reopen the associated editor window.

Enhancements:

  • Move RemoteEdit ownership from Proxy to MainWindow and wire Proxy–RemoteEdit communication via signals and slots.
  • Track RemoteEdit sessions as async tasks to support cancellation, progress reporting, and re-raising associated editor windows.
  • Unify internal and external editor sessions around shared draft file paths, deferring deletion of edit-session files until delivery is considered complete.

- Consolidate MPI session and draft management into RemoteEdit
- Decouple Proxy from RemoteEdit using signals/slots
- Move RemoteEdit ownership to MainWindow
- Implement draft file persistence in MMapper/Editor/
- Implement atomic auto-save with debounce (2s) and throttle (15s)
- Implement draft recovery scan at startup
- Integrate RemoteEdit tasks with TasksPanel and AsyncTask system
- Add 'Show Editor' functionality for active and recovered tasks
@google-labs-jules

Copy link
Copy Markdown

👋 Jules, reporting for duty! I'm here to lend a hand with this pull request.

When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down.

I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job!

For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with @jules. You can find this option in the Pull Request section of your global Jules UI settings. You can always switch back!

New to Jules? Learn more at jules.google/docs.


For security, I will only act on instructions from the user who triggered this task.

@sourcery-ai

sourcery-ai Bot commented Jul 5, 2026

Copy link
Copy Markdown

Reviewer's Guide

Refactors RemoteEdit into a MainWindow-owned, long-lived draft manager with async task integration, persistent draft files, and startup recovery of orphaned drafts, while decoupling Proxy via signals/slots and wiring UI for auto-save and recovered-draft viewing.

Sequence diagram for RemoteEdit draft lifecycle and recovery

sequenceDiagram
    actor User
    participant MF as MpiFilterToMud
    participant PX as Proxy
    participant MW as MainWindow
    participant RE as RemoteEdit
    participant SES as RemoteEditSession
    participant AT as AsyncTasks
    participant TP as TasksPanel

    MF->>PX: slot_remoteEdit(id, title, body)
    PX->>RE: sig_remoteEditRequested(sessionId, title, body)
    RE->>RE: addSession(sessionId, title, body)
    RE->>RE: provisionDraftFile(sessionId, title, body)
    RE->>SES: setDraftFileName(fileName)
    RE->>AT: startAsyncTask(RemoteEdit,...)
    AT-->>RE: AsyncTaskHandle
    RE->>SES: setAsyncTask(handle)

    rect rgb(230,230,255)
        User->>TP: click "Show Editor" for RemoteEdit task
        TP->>MW: getRemoteEdit()
        MW->>RE: raiseSession(taskId)
        RE->>SES: getSessionByTaskId(taskId)
        SES->>SES: raise()
    end

    rect rgb(230,255,230)
        MW->>RE: recoverDrafts()
        RE->>RE: discoverDrafts()
        RE->>SES: create recovered RemoteEditSession(...)
        RE->>AT: startAsyncTask(RemoteEdit,...)
        AT-->>RE: AsyncTaskHandle
        RE->>SES: setAsyncTask(handle)
        RE->>SES: setDraftFileName(...)
        RE->>SES: setDisconnected()
    end

    rect rgb(255,230,230)
        SES->>RE: save()
        RE->>PX: sig_remoteEditSave(sessionId, content)
        PX->>MF: saveRemoteEdit(sessionId, content)
        RE->>RE: deleteDraft(draftFileName)
    end
Loading

File-Level Changes

Change Details Files
Centralize RemoteEdit lifetime and decouple it from Proxy via signals/slots.
  • MainWindow now owns a RemoteEdit instance and wires it to newly created Proxy objects via ConnectionListener signals
  • Proxy no longer creates or stores RemoteEdit; instead exposes signals for remote edit/view requests and slots for save/cancel operations
  • GameObserver exposes a disconnected signal used by MainWindow to notify RemoteEdit on mud disconnect
  • ConnectionListener emits sig_proxyCreated and provides access to the Proxy instance
src/mainwindow/mainwindow.cpp
src/mainwindow/mainwindow.h
src/proxy/proxy.cpp
src/proxy/proxy.h
src/proxy/connectionlistener.cpp
src/proxy/connectionlistener.h
src/observer/gameobserver.cpp
src/observer/gameobserver.h
Introduce persistent draft file management, metadata encoding, and recovery of orphaned drafts.
  • RemoteEdit now manages sessions via shared_ptr and tracks async RemoteEdit tasks per session
  • Draft files are provisioned when edit sessions are created, with metadata-encoded filenames and a dedicated editor directory from configuration
  • Added atomic draft save and delete helpers, plus discovery of existing draft files
  • recoverDrafts scans the draft directory at startup, registers recovered sessions as disconnected edit tasks, and exposes raiseSession/getSessionByTaskId for UI integration
src/mpi/remoteedit.cpp
src/mpi/remoteedit.h
src/configuration/configuration.cpp
src/configuration/configuration.h
Integrate RemoteEdit sessions with AsyncTasks and the TasksPanel for long-lived tracking and UI actions.
  • AsyncTaskTypeEnum gains a RemoteEdit type and type name mapping
  • Edit and recovered sessions start a RemoteEdit async task that keeps running until cancelled or session removal
  • RemoteEditSession stores AsyncTaskHandle and draft filename and exposes shouldStopTask/stopTask
  • TasksPanel ListItem is extended to accept MainWindow, and adds a "Show Editor" button for RemoteEdit tasks that raises the associated editor window via RemoteEdit::raiseSession
src/global/AsyncTasks.cpp
src/global/AsyncTasks.h
src/mpi/remoteedit.cpp
src/mpi/remoteeditsession.h
src/mainwindow/TasksPanel.cpp
Add auto-save with debounce/throttle and read-only viewing for recovered drafts in the internal editor, plus external editor integration with persistent files.
  • RemoteEditInternalSession now tracks title, draft filename, and timers for debounced and throttled auto-save, writing drafts via RemoteEdit::saveDraftAtomic
  • RemoteEditWidget emits sig_textModified when text changes, wired to the internal session for auto-save
  • RemoteEditSession gains a raise() virtual for bringing up editors, with a concrete implementation showing recovered drafts in a read-only RemoteEditWidget
  • RemoteEditExternalSession and RemoteEditProcess now operate on a provided full draft path, reuse existing files when present, and avoid deleting edit-session files so they can be managed by RemoteEdit; view-only sessions still use temp files and clean them up on destruction
src/mpi/remoteeditsession.cpp
src/mpi/remoteeditsession.h
src/mpi/remoteeditwidget.cpp
src/mpi/remoteeditwidget.h
src/mpi/remoteeditprocess.cpp
src/mpi/remoteeditprocess.h
Extend configuration with a dedicated editor directory used for draft storage.
  • Added KEY_EDITOR_DIRECTORY and DEFAULT_EDITOR_SUBDIR constants and persisted editorDirectory in MumeClientProtocolSettings
  • Editor directory defaults to a subfolder under the main MMapper directory and is created when used by RemoteEdit
src/configuration/configuration.cpp
src/configuration/configuration.h

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey - I've found 2 issues, and left some high level feedback:

  • The async task loop in RemoteEdit::addSession/RemoteEdit::recoverDrafts relies on RemoteEditSession::shouldStopTask without any synchronization, which can lead to data races between the worker thread and the GUI thread; consider using an atomic flag or another thread-safe mechanism for signalling task termination.
  • RemoteEditExternalSession currently passes getFullDraftPath() (which is empty at construction time) into RemoteEditProcess, so external edit sessions fall back to a temp file instead of the provisioned draft path; you likely want to provision the draft and set m_draftFileName before constructing RemoteEditExternalSession so the external editor uses the persistent draft file.
  • In connectionlistener.h, replacing the forward declaration of Proxy with a local #include "proxy.h" inside the forward-declarations block introduces a tighter header coupling and potential circular-dependency issues; it would be cleaner to restore the forward declaration and move the include to the top of the file only where a full definition is needed.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- The async task loop in RemoteEdit::addSession/RemoteEdit::recoverDrafts relies on RemoteEditSession::shouldStopTask without any synchronization, which can lead to data races between the worker thread and the GUI thread; consider using an atomic flag or another thread-safe mechanism for signalling task termination.
- RemoteEditExternalSession currently passes getFullDraftPath() (which is empty at construction time) into RemoteEditProcess, so external edit sessions fall back to a temp file instead of the provisioned draft path; you likely want to provision the draft and set m_draftFileName before constructing RemoteEditExternalSession so the external editor uses the persistent draft file.
- In connectionlistener.h, replacing the forward declaration of Proxy with a local #include "proxy.h" inside the forward-declarations block introduces a tighter header coupling and potential circular-dependency issues; it would be cleaner to restore the forward declaration and move the include to the top of the file only where a full definition is needed.

## Individual Comments

### Comment 1
<location path="src/mpi/remoteedit.cpp" line_range="74-83" />
<code_context>
+    if (isEdit) {
</code_context>
<issue_to_address>
**issue (bug_risk):** Async task lambda holds a raw session pointer, which can dangle when the session is removed.

In `addSession()`, the RemoteEdit async task captures `auto* pSession = session.get()` by value and uses it in a long-running loop. In `removeSession()`, `stopTask()` is called and the session is erased from `m_sessions`, destroying the `shared_ptr` and the session object while the async thread may still access `pSession->shouldStopTask()`, leading to undefined behavior. The recovered-drafts path correctly captures a `std::shared_ptr` and uses `get()` only inside the lambda. Please change the regular edit path to capture a `std::shared_ptr<RemoteEditSession>` (or otherwise ensure the task owns the session and the session outlives the task).
</issue_to_address>

### Comment 2
<location path="src/mpi/remoteedit.cpp" line_range="346-349" />
<code_context>
+    return false;
+}
+
+void RemoteEdit::deleteDraft(const QString &fileName)
+{
+    if (fileName.isEmpty()) return;
+    QFile::remove(QDir(getDraftDirectory()).absoluteFilePath(fileName));
+}
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** deleteDraft silently ignores failure to remove the draft file.

If the draft directory isn’t writable or the file can’t be removed, `deleteDraft` fails silently. Since drafts drive recovery (`discoverDrafts`), leftover files could cause confusing behaviour (e.g. tasks “recovered” after being cancelled). Please log a warning when `QFile::remove` returns false to aid diagnosing editorDirectory or filesystem permission issues.

Suggested implementation:

```cpp
void RemoteEdit::deleteDraft(const QString &fileName)
{
    if (fileName.isEmpty()) {
        return;
    }

    const QString draftPath = QDir(getDraftDirectory()).absoluteFilePath(fileName);
    QFile draftFile(draftPath);

    if (!draftFile.remove()) {
        qWarning() << "RemoteEdit::deleteDraft: failed to remove draft file"
                   << draftPath
                   << "- error:" << draftFile.errorString();
    }
}

```

If `qWarning` or `QFile`/`QDir` are not yet included in this translation unit, you may need to ensure the appropriate Qt headers are present (typically `<QFile>`, `<QDir>`, and, if required by your logging setup, `<QDebug>` or `<QtGlobal>`). Align the logging style with the rest of the file (e.g. if it uses a custom logging helper or `QLoggingCategory`, adapt the `qWarning` call accordingly).
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Comment thread src/mpi/remoteedit.cpp Outdated
Comment thread src/mpi/remoteedit.cpp
Comment on lines +346 to +349
void RemoteEdit::deleteDraft(const QString &fileName)
{
if (fileName.isEmpty()) return;
QFile::remove(QDir(getDraftDirectory()).absoluteFilePath(fileName));

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (bug_risk): deleteDraft silently ignores failure to remove the draft file.

If the draft directory isn’t writable or the file can’t be removed, deleteDraft fails silently. Since drafts drive recovery (discoverDrafts), leftover files could cause confusing behaviour (e.g. tasks “recovered” after being cancelled). Please log a warning when QFile::remove returns false to aid diagnosing editorDirectory or filesystem permission issues.

Suggested implementation:

void RemoteEdit::deleteDraft(const QString &fileName)
{
    if (fileName.isEmpty()) {
        return;
    }

    const QString draftPath = QDir(getDraftDirectory()).absoluteFilePath(fileName);
    QFile draftFile(draftPath);

    if (!draftFile.remove()) {
        qWarning() << "RemoteEdit::deleteDraft: failed to remove draft file"
                   << draftPath
                   << "- error:" << draftFile.errorString();
    }
}

If qWarning or QFile/QDir are not yet included in this translation unit, you may need to ensure the appropriate Qt headers are present (typically <QFile>, <QDir>, and, if required by your logging setup, <QDebug> or <QtGlobal>). Align the logging style with the rest of the file (e.g. if it uses a custom logging helper or QLoggingCategory, adapt the qWarning call accordingly).

@codecov

codecov Bot commented Jul 5, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 1.77515% with 332 lines in your changes missing coverage. Please review.
✅ Project coverage is 26.07%. Comparing base (1e3eafb) to head (91d963b).

Files with missing lines Patch % Lines
src/mpi/remoteedit.cpp 0.00% 182 Missing ⚠️
src/mpi/remoteeditsession.cpp 0.00% 46 Missing ⚠️
src/proxy/proxy.cpp 0.00% 29 Missing ⚠️
src/mpi/remoteeditprocess.cpp 0.00% 23 Missing ⚠️
src/mainwindow/mainwindow.cpp 0.00% 15 Missing ⚠️
src/mainwindow/TasksPanel.cpp 0.00% 10 Missing ⚠️
src/mpi/remoteeditsession.h 0.00% 7 Missing ⚠️
src/proxy/MudTelnet.cpp 0.00% 5 Missing ⚠️
src/mpi/remoteeditwidget.cpp 0.00% 4 Missing ⚠️
src/proxy/MudTelnet.h 0.00% 4 Missing ⚠️
... and 4 more
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #241      +/-   ##
==========================================
+ Coverage   25.08%   26.07%   +0.98%     
==========================================
  Files         528      528              
  Lines       44211    45812    +1601     
  Branches     4793     4847      +54     
==========================================
+ Hits        11092    11944     +852     
- Misses      33119    33868     +749     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

nschimme added 6 commits July 6, 2026 03:59
- Centralize MPI session and draft management in RemoteEdit
- MainWindow now owns the long-lived RemoteEdit instance
- Decouple Proxy from RemoteEdit via direct GMCP subscription in RemoteEdit
- Mark all active and recovered edits as AsyncTasks for unified management
- Implement draft persistence in MMapper/Editor directory
- Implement atomic auto-save with debounce (2s) and throttle (15s)
- Add draft recovery scan at startup and immediately after disconnects
- Integrate 'Show Editor' in TasksPanel to raise windows or open read-only drafts
- Ensure drafts are purged only upon confirmed write/cancel from server
- Handle clean and unclean disconnects to preserve draft state
- Establish MMapper/Editor/ scratch directory for draft persistence.
- Centralize RemoteEdit management in MainWindow and decouple from Proxy via GMCP signals.
- Implement atomic auto-save with 2000ms debounce and 15000ms throttle.
- Add startup recovery sequence to scan for orphaned draft files.
- Ensure thread safety in background tasks using weak_ptr for session access.
- Preserve draft files across disconnections until explicit server confirmation.
- Update TasksPanel with "Show Editor" functionality for active and recovered drafts.
- Enhance UX for external editor tasks with status notifications.
- Add virtual isRunning() to RemoteEditSession base class.
- Override isRunning() in RemoteEditExternalSession to check process state.
- Use virtual isRunning() in RemoteEdit::raiseSession to avoid conditional compilation issues with external sessions.
- Fix variable shadowing in slot_parseGmcpInput by renaming local error message variable.
- Ensure consistent behavior across all platforms including Wasm and Snap.
- Establish MMapper/Editor/ scratch directory for draft persistence.
- Centralize RemoteEdit management in MainWindow and decouple from Proxy via GMCP signals.
- Implement atomic auto-save with 2000ms debounce and 15000ms throttle.
- Add startup recovery sequence to scan for orphaned draft files.
- Ensure thread safety in background tasks using weak_ptr for session access.
- Preserve draft files across disconnections until explicit server confirmation.
- Transition disconnected closed sessions to recovered state in the task list.
- Simplified UX: Removed 'raise' feature and clipboard copy on disconnect.
- Centralize RemoteEdit management in MainWindow and decouple from Proxy.
- Route MUME.Client GMCP messages via Proxy signals for protocol stability.
- Implement atomic auto-saving with 2s debounce and 15s throttle using QSaveFile.
- Add recovery logic for orphaned drafts on application startup.
- Update TasksPanel with a functional UI to view and extract recovered drafts.
- Fulfills functional requirements FR-0.1 through FR-6.4.
- Removed stale connection to m_remoteEdit->slot_parseGmcpInput in MainWindow.
- Decoupled RemoteEdit result handling from generic GMCP parsing.
- Added virtual methods to MudTelnetOutputs for remote write/cancel results.
- Proxy now emits specific signals for remote edit results.
- RemoteEdit now deletes drafts only upon confirmed server success via slot_remoteWriteResult.
- This fixes the CI failure where RemoteEdit had no member slot_parseGmcpInput.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant